feat: support when_matched_delete in merge_insert #4939

jtuglu1 · 2025-10-13T05:30:59Z

codecov-commenter · 2025-10-14T02:50:57Z

Codecov Report

❌ Patch coverage is 86.30807% with 56 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...ance/src/dataset/write/merge_insert/exec/delete.rs	78.50%	33 Missing and 13 partials ⚠️
rust/lance/src/dataset/write/merge_insert.rs	94.20%	8 Missing ⚠️
...lance/src/dataset/write/merge_insert/exec/write.rs	90.90%	0 Missing and 1 partial ⚠️
...nce/src/dataset/write/merge_insert/logical_plan.rs	92.85%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

jtuglu1 · 2025-10-14T16:37:08Z

@wjones127 I plan to push some changes to bump the coverage a bit. Lack of coverage is from explain/analyze plan methods. Something I think would be a good idea to future-proof this file is starting to standardize how these various combinations of configs should be unit tested together. We can always attempt to test all relevant combinations with targeted tests, but not sure how scalable that is. Maybe there's a better approach with parameterized tests, etc.?

rust/lance/src/dataset/write/merge_insert.rs

wjones127 · 2025-10-14T17:28:30Z

Maybe there's a better approach with parameterized tests, etc.?

There's probably a better parametrized test. I'm working on adding a larger test suite, which can cover more write cases.

python/python/lance/dataset.py

rust/lance/src/dataset/write/merge_insert.rs

jackye1995

mostly looks good to me, some minor comments

jtuglu1 · 2025-12-30T20:19:12Z

@wjones127 @jackye1995 thoughts here?

jackye1995 · 2025-12-31T23:24:53Z

@jtuglu1 I thought about this a bit more, and also saw Will's old comment (sorry totally missed that previously) I think what Will suggested makes sense, I pushed a commit to add DeleteOnlyMergeInsertExec with some additional refactoring, let me know if this looks good to you

jtuglu1 · 2025-12-31T23:27:46Z

Yeah I considered this (and Will's comment), but I didn't really like the idea of adding physical plans for every single type of enum (.*MergeInsertExec). It seemed easier (to me) to handle things all in the same codepath (the projection optimizations, etc.). I haven't really looked through your changes here yet, but are there any performance benefits as compared to the current implementation?

Edit: looks like this is skipping write step which is I guess some time saved.

jackye1995 · 2025-12-31T23:57:21Z

Edit: looks like this is skipping write step which is I guess some time saved.

yes that's the main goal

jackye1995

thanks for working through this!

rust/lance/src/dataset/write/merge_insert/exec/delete.rs

github-actions bot added enhancement New feature or request java labels Oct 13, 2025

jtuglu1 force-pushed the add-when-match-delete-functionality branch 3 times, most recently from db7cd37 to 925f954 Compare October 14, 2025 02:03

github-actions bot added the python label Oct 14, 2025

jtuglu1 force-pushed the add-when-match-delete-functionality branch 3 times, most recently from 4898546 to b793aba Compare October 14, 2025 03:42

jtuglu1 marked this pull request as ready for review October 14, 2025 04:35

wjones127 self-assigned this Oct 14, 2025

wjones127 reviewed Oct 14, 2025

View reviewed changes

rust/lance/src/dataset/write/merge_insert.rs Outdated Show resolved Hide resolved

jtuglu1 force-pushed the add-when-match-delete-functionality branch 3 times, most recently from d1b0c26 to b17144d Compare December 29, 2025 22:55

Refactor

13fac77

jtuglu1 requested a review from wjones127 December 30, 2025 03:55